Skip to content

tools: add benchmark for loading sets#535

Merged
qdeslandes merged 1 commit into
facebook:mainfrom
pzmarzly:push-voqxqossrrll
May 13, 2026
Merged

tools: add benchmark for loading sets#535
qdeslandes merged 1 commit into
facebook:mainfrom
pzmarzly:push-voqxqossrrll

Conversation

@pzmarzly
Copy link
Copy Markdown
Contributor

Measure time it takes to load a chain that contains a large set. This will be a common operation (especially since sets can update).

Requires #532.

$ rm -rf build && cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=release -DNO_DOCS=1 -DNO_CHECKS=1
$ ninja -C build benchmark_bin bfcli
$ sudo build/output/sbin/benchmark_bin \
    --cli build/output/sbin/bfcli \
    --srcdir . \
    --outfile /tmp/load.json \
    --filter chain_load

...
Run on (4 X 2396.4 MHz CPU s)
...
chain_load__ip4_saddr__x_elem_set/8/manual_time           7.20 ms         7.08 ms          106 load chain, ip4.saddr, 8 elements set
chain_load__ip4_saddr__x_elem_set/128/manual_time         7.69 ms         7.61 ms          107 load chain, ip4.saddr, 128 elements set
chain_load__ip4_saddr__x_elem_set/32768/manual_time       55.3 ms         61.5 ms           12 load chain, ip4.saddr, 32768 elements set

@pzmarzly pzmarzly requested a review from qdeslandes as a code owner May 12, 2026 17:19
@meta-cla meta-cla Bot added the cla signed label May 12, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 12, 2026

Claude review of PR #535 (5e6bef2)

Suggestions

  • SetIterationTime before error check (code restructured; no longer uses SetIterationTime)

Nits

  • Keyword-only params for Stats.failure() (bfbencher file no longer in PR)
  • Document commits ordering assumption (bfbencher file no longer in PR)
  • Comment on iteration pattern divergence (dismissed by maintainer: for (auto _: state) is correct)
  • camelCase local variabletools/benchmarks/main.cpp:212 — fixed: now uses chain_name
  • Set/Rule insertion ordertools/benchmarks/main.cpp:219 — existing set benchmarks add Rule before Set; this one reverses the order (functionally equivalent but inconsistent)

Workflow run

@pzmarzly pzmarzly force-pushed the push-voqxqossrrll branch from 66e1e63 to 64c0686 Compare May 12, 2026 17:27
Comment thread tools/benchmarks/main.cpp
Comment thread tools/benchmarks/bfbencher
Comment thread tools/benchmarks/bfbencher
Comment thread tools/benchmarks/main.cpp Outdated
@qdeslandes qdeslandes force-pushed the push-voqxqossrrll branch from 64c0686 to 34cef1d Compare May 12, 2026 20:02
Comment thread tools/benchmarks/main.cpp Outdated
Comment thread tools/benchmarks/main.cpp Outdated
Comment on lines +243 to +247
ret = bf_chain_flush(chainName.c_str());
if (ret < 0) {
state.SkipWithError("failed to flush chain");
break;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use

{
    benchmark::ScopedPauseTiming pause(state); // Pauses timing
    ret = bf_chain_flush(chainName.c_str());
    if (ret < 0) {
        state.SkipWithError("failed to flush chain");
        break;
    }
}

So you don't have to manually set the iteration time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ScopedPauseTiming was only merged a month ago, and is not available yet in gbenchmark shipped by my distro. Fun! google/benchmark#2157

I'll use PauseTiming and ResumeTiming functions.

Comment thread tools/benchmarks/main.cpp Outdated
Comment thread tools/benchmarks/main.cpp Outdated
Comment thread tools/benchmarks/main.cpp Outdated
@pzmarzly pzmarzly force-pushed the push-voqxqossrrll branch from 34cef1d to fc95024 Compare May 13, 2026 14:54
Comment thread tools/benchmarks/main.cpp Outdated
@pzmarzly pzmarzly force-pushed the push-voqxqossrrll branch from fc95024 to 5e6bef2 Compare May 13, 2026 15:16
@pzmarzly
Copy link
Copy Markdown
Contributor Author

@claude review

Comment thread tools/benchmarks/main.cpp
@qdeslandes qdeslandes enabled auto-merge (rebase) May 13, 2026 15:33
Copy link
Copy Markdown
Contributor

@qdeslandes qdeslandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

@qdeslandes qdeslandes merged commit ff16870 into facebook:main May 13, 2026
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants